Fix empty list handling for tool filters in session methods#487
Fix empty list handling for tool filters in session methods#487Halcyonhal9 wants to merge 2 commits intogithub:mainfrom
Conversation
…t being able to set a null list, the default 14 tools that the cli ships with are present. Sometimes you need toolless calls. Same pattern exists on the excluded_tools variable. From a quick look, there are a lot of incorrect null checks that should be tighterned up. I don't know what the functional impact is, so sticking with the case that is causing a known bug.
There was a problem hiding this comment.
Pull request overview
Fixes Python SDK session creation/resume payload construction so callers can explicitly pass empty tool filter lists (e.g., available_tools: []) without them being dropped by truthy checks, enabling fully “toolless” sessions when desired.
Changes:
- Update
create_session()to treatavailable_tools/excluded_toolsas “provided” when they are notNone(including empty lists). - Update
resume_session()with the sameis not Nonehandling for tool filters.
| available_tools = cfg.get("available_tools") | ||
| if available_tools: | ||
| if available_tools is not None: | ||
| payload["availableTools"] = available_tools | ||
| excluded_tools = cfg.get("excluded_tools") | ||
| if excluded_tools: | ||
| if excluded_tools is not None: | ||
| payload["excludedTools"] = excluded_tools |
There was a problem hiding this comment.
This change fixes the empty-list case for available_tools/excluded_tools, but there isn’t a regression test that asserts an explicit empty list is preserved and results in a toolless request. Consider adding an E2E test case (similar to test_should_create_a_session_with_availableTools) that calls create_session({"available_tools": []}) and verifies the recorded request contains no tools.
| available_tools = cfg.get("available_tools") | ||
| if available_tools: | ||
| if available_tools is not None: | ||
| payload["availableTools"] = available_tools | ||
|
|
||
| excluded_tools = cfg.get("excluded_tools") | ||
| if excluded_tools: | ||
| if excluded_tools is not None: | ||
| payload["excludedTools"] = excluded_tools |
There was a problem hiding this comment.
resume_session now correctly forwards empty tool filter lists, but current tests appear to cover only create_session tool filters. Add a regression test that resumes a session with available_tools: [] (or excluded_tools: []) and asserts the subsequent request has no tools, to prevent this from regressing again.
|
@copilot open a new pull request to apply changes based on the comments in this thread |
1 participant
Fix null checks for available_tools and excluded_tools in create_session and resume_session
Both create_session and resume_session use truthy checks for the available_tools and config options:
if available_tools:
if excluded_tools:
For available_tools this silently drops empty lists ([]), which prevents callers from explicitly clearing the default 14 built-in CLI tools to make toolless calls.
Changes
python/copilot/client.py: Change available_tools and excluded_tools in create_session and resume_session to
if available_tools is not None:
if excluded_tools is not None:
Test results
pytest: 111 passed, 5 skipped
ruff check: all checks passed